WIP: Update GN flags to properly do Widevine Host Verification / signing #515

bsclifton · 2018-09-27T05:39:12Z

Current Status

This PR (as-is) works great on macOS (including the generation of the .sig file). Of course, you need to use brave/brave-browser#1320 from brave-browser when testing this PR

Windows Status

Windows is not working and I have discovered why (but I am not sure how to fix)

non-Windows builds register the UtilityServiceFactory

On macOS (and Linux), this code fires:
https://cs.chromium.org/chromium/src/content/app/content_main_runner_impl.cc?l=508-509&rcl=874d2e3f4036268425593cea23a0ae9b1b00e07f

 UtilityProcessHost::RegisterUtilityMainThreadFactory(
      CreateInProcessUtilityThread);

UtilityServiceFactory registers implementation for CdmService

This (through a few steps) launches the utility process, which creates an instance of UtilityServiceFactory. This will then register the CdmService interface:
https://cs.chromium.org/chromium/src/content/utility/utility_service_factory.cc?l=154-156&rcl=5f75c871d1ec0f3ff08650dcb87e361daf431705

  service_manager::EmbeddedServiceInfo info;
  info.factory = base::Bind(&CreateCdmService);
  services->emplace(media::mojom::kCdmServiceName, info);

The implementation for CdmService provides the host verification

The important part (ex: host verification) happens when this CdmService is implemented, it'll provide a method to check the .sig files. You can view that here:
https://cs.chromium.org/chromium/src/content/utility/utility_service_factory.cc?l=96-99&rcl=741e7070889303f4cf376dad52ae67aec69889fe

void AddCdmHostFilePaths(
      std::vector<media::CdmHostFilePath>* cdm_host_file_paths) override {
    GetContentClient()->AddContentDecryptionModules(nullptr,
                                                    cdm_host_file_paths);
}

What exactly is broken on Windows?

Going back to the part that fails on Windows. When a page loads that uses Widevine, it creates a media player and then tries (but is unable) to find an instance of the CdmService:
https://cs.chromium.org/chromium/src/content/browser/media/media_interface_proxy.cc?l=342-346&rcl=aa20726d6256b54ba793eb290545339f49b05113

service_manager::Connector* connector =
    ServiceManagerConnection::GetForProcess()->GetConnector();

media::mojom::CdmServicePtr cdm_service;
connector->BindInterface(identity, &cdm_service);

This means subsequent calls to cdm_service are no-ops:
https://cs.chromium.org/chromium/src/content/browser/media/media_interface_proxy.cc?rcl=aa20726d6256b54ba793eb290545339f49b05113&l=357

cdm_service->LoadCdm(cdm_path);

What is left? Where is help needed?

We need to register the CdmService on Windows (other platforms are OK because they launch the utility process). I've tried a few ways to patch this, but haven't had any success.

One thing worth noting:
AFTER the page is finished loading, if I wait long enough... I eventually DO see a Utility process launch. A new process is launched with utility as an argument which gets picked up here:
https://cs.chromium.org/chromium/src/content/app/content_main_runner_impl.cc?rcl=874d2e3f4036268425593cea23a0ae9b1b00e07f&l=431-433

if (process_type == switches::kUtilityProcess ||
        cmd->HasSwitch(switches::kSingleProcess))
      content_client->utility_ = delegate->CreateContentUtilityClient();

This is created via UtilityProcessHost. I was unable to find where Windows is creating this. The fix might be "as easy" as creating an instance of this on browser-launch (on Windows)
https://cs.chromium.org/chromium/src/content/browser/utility_process_host.cc?l=273&rcl=53453fb02b81803f64293e3002c6765286a429dd

bool UtilityProcessHost::StartProcess() {

Why does Chrome work?

I haven't done enough debugging to provide a solid answer on this... However, my best guess is that this works because some additional flags are enabled for Chrome (set to true):

enable_rlz
should_bundle_widevine_cdm
enable_cdm_storage_id

One potential solution would be to enable each of those and then stub out the RLZ implementation

Description

Fixes brave/brave-browser#944
Fixes brave/brave-browser#1310

Requires brave/brave-browser#1320

NOTE: After this is approved / merged, we'll need to open an issue in the devops repo to track adding the new brave_enable_cdm_host_verification param to the .npmrc on the CI servers

Submitter Checklist:

Submitted a ticket for my issue if one did not already exist.
Used Github auto-closing keywords in the commit message.
Added/updated tests for this change (for new code or code which already has tests).
Ran git rebase -i to squash commits (if needed).
Tagged reviewers and labelled the pull request as needed.
Request a security/privacy review as needed.
Add appropriate QA labels (QA/Needed or QA/No-QA-Needed) to include the closed issue in milestone

Test Plan:

Reviewer Checklist:

New files have MPL-2.0 license header.
Request a security/privacy review as needed.
Adequate test coverage exists to prevent regressions
Verify test plan is specified in PR before merging to source

If set to `true` via .npmrc, Widevine Host Verification will be enabled for the build This functionality also requires brave/brave-core#515

If set to `true` via .npmrc, Widevine Host Verification will be enabled for the build This functionality also requires brave/brave-core#515 Fixes #1310

darkdh · 2018-09-27T16:19:45Z

patches/third_party-widevine-cdm-widevine.gni.patch

+diff --git a/third_party/widevine/cdm/widevine.gni b/third_party/widevine/cdm/widevine.gni
+index 82a93622585a424f56c3567050f9ba4822de6c1e..9c89d9a0c0a8b2df4e082cfd1ef9ace6ed8fc929 100644
+--- a/third_party/widevine/cdm/widevine.gni
+++ b/third_party/widevine/cdm/widevine.gni


we don't need third_party/widevine because we don't bundle widevine cdm.

nvm, just saw enable_widevine_cdm_host_verification also used in chrome/BUILD.gn for generating widevine resource bundle and signing framework instead of just used only with should_bundle_widevine_cdm

please rebase your patch to 70.0.3538.22 so we can get rid of this patch

13 enable_widevine_cdm_host_verification = 14 enable_widevine && enable_cdm_host_verification 15 16 # Only bundle Widevine CDM in Google Chrome builds. 17 should_bundle_widevine_cdm = is_chrome_branded && enable_library_cdms

it turns out I have previously local change applied and this patch will be needed to remov in the future. https://chromium.googlesource.com/chromium/src/+/f170c737755811ca5d7ded0700d8aa17ec310024
but we still need this patch for now

darkdh · 2018-09-27T16:35:30Z

patches/third_party-widevine-cdm-widevine.gni.patch

+diff --git a/third_party/widevine/cdm/widevine.gni b/third_party/widevine/cdm/widevine.gni
+index 82a93622585a424f56c3567050f9ba4822de6c1e..9c89d9a0c0a8b2df4e082cfd1ef9ace6ed8fc929 100644
+--- a/third_party/widevine/cdm/widevine.gni
+++ b/third_party/widevine/cdm/widevine.gni


please rebase your patch to 70.0.3538.22 so we can get rid of this patch

13 enable_widevine_cdm_host_verification = 14 enable_widevine && enable_cdm_host_verification 15 16 # Only bundle Widevine CDM in Google Chrome builds. 17 should_bundle_widevine_cdm = is_chrome_branded && enable_library_cdms

darkdh · 2018-09-28T16:28:20Z

@bsclifton you also need to change chrome/common/media/cdm_host_file_path.cc

#if defined(GOOGLE_CHROME_BUILD)
...
void AddCdmHostFilePaths(
    std::vector<media::CdmHostFilePath>* cdm_host_file_paths) {
...
}
#else  // defined(GOOGLE_CHROME_BUILD)
void AddCdmHostFilePaths(
    std::vector<media::CdmHostFilePath>* cdm_host_file_paths) {
  NOTIMPLEMENTED() << "CDM host file paths need to be provided for the CDM to "
                      "verify the host.";
}

#endif  // defined(GOOGLE_CHROME_BUILD)

bsclifton · 2018-09-28T17:49:36Z

@darkdh done! 😄👍

If set to `true` via .npmrc, Widevine Host Verification will be enabled for the build This functionality also requires brave/brave-core#515 Fixes #1310

bbondy · 2018-10-11T16:56:55Z

build/BUILD.gn

@@ -20,4 +20,4 @@ config("version") {
  defines = [
    "BRAVE_CHROMIUM_VERSION=\"$chrome_version_string\"",
  ]
-}


You can probably just leave out this file change from the commit so its history doesn't change.

bbondy · 2018-10-11T16:59:07Z

patches/chrome-common-media-cdm_host_file_path.cc.patch

+ #endif
+
+-#if defined(GOOGLE_CHROME_BUILD)
+#if defined(GOOGLE_CHROME_BUILD) || defined(BRAVE_CHROMIUM_BUILD)


you can just use a chromium_src override for this where you include the base path but first define GOOGLE_CHROME_BUILD and undef it afterwards.

bsclifton · 2018-10-12T23:33:15Z

patches/content-browser-media-media_interface_proxy.cc.patch

+  ServiceMap services;
+  factory.RegisterServices(&services);
+  // TODO: register with service map for this thread?
+#else


This can be removed when we revisit; was just a bad attempt at trying to get this working

- #507 - #510 This code was backed out with #513 (so we can do 0.55.x and 0.56.x builds in the meantime). This functionality also requires brave/brave-browser#1320

If set to `true` via .npmrc, Widevine Host Verification will be enabled for the build This functionality also requires brave/brave-core#515 Fixes #1310

bbondy · 2018-12-02T15:25:26Z

Ping since this is ~1 month old, can this be closed?

mihaiplesa · 2019-04-03T09:00:59Z

Closing as work has been done in #2023

bsclifton added the pr/work-in-progress label Sep 27, 2018

bsclifton self-assigned this Sep 27, 2018

bsclifton force-pushed the readd-widevine-gn-support branch from bf59daa to 622e34d Compare September 27, 2018 05:53

bsclifton mentioned this pull request Sep 27, 2018

WIP: Add check for new brave_enable_cdm_host_verification config option brave/brave-browser#1320

Closed

16 tasks

bsclifton force-pushed the readd-widevine-gn-support branch from 995081f to 4f71dbd Compare September 27, 2018 15:54

bsclifton requested review from darkdh, mihaiplesa and yrliou September 27, 2018 15:54

bsclifton added a commit to brave/brave-browser that referenced this pull request Sep 27, 2018

For testing with CI, set the brave-core branch to brave/brave-core#515

97caff7

darkdh requested changes Sep 27, 2018

View reviewed changes

mihaiplesa requested a review from simonhong October 1, 2018 15:44

bsclifton assigned mihaiplesa Oct 1, 2018

bsclifton force-pushed the readd-widevine-gn-support branch from ec0985d to 9157cce Compare October 1, 2018 19:48

bsclifton added a commit to brave/brave-browser that referenced this pull request Oct 1, 2018

For testing with CI, set the brave-core branch to brave/brave-core#515

5f47dc9

bsclifton force-pushed the readd-widevine-gn-support branch 2 times, most recently from 39b0710 to 72e1e48 Compare October 2, 2018 00:54

mihaiplesa pushed a commit to brave/brave-browser that referenced this pull request Oct 2, 2018

For testing with CI, set the brave-core branch to brave/brave-core#515

bd495a0

bsclifton added a commit to brave/brave-browser that referenced this pull request Oct 2, 2018

For testing with CI, set the brave-core branch to brave/brave-core#515

ee3babc

bsclifton force-pushed the readd-widevine-gn-support branch 2 times, most recently from 8734b31 to 3f718ae Compare October 2, 2018 18:58

bsclifton added a commit to brave/brave-browser that referenced this pull request Oct 2, 2018

For testing with CI, set the brave-core branch to brave/brave-core#515

a51436c

bsclifton force-pushed the readd-widevine-gn-support branch 13 times, most recently from 18fc6d0 to e8f9671 Compare October 11, 2018 05:16

bbondy reviewed Oct 11, 2018

View reviewed changes

bsclifton changed the title ~~Update GN flags to properly do Widevine Host Verification / signing~~ WIP: Update GN flags to properly do Widevine Host Verification / signing Oct 11, 2018

bsclifton commented Oct 12, 2018

View reviewed changes

bsclifton and others added 2 commits October 13, 2018 21:32

Re-implement work done originally done with:

681d136

- #507 - #510 This code was backed out with #513 (so we can do 0.55.x and 0.56.x builds in the meantime). This functionality also requires brave/brave-browser#1320

added widevine signature to dependencies

afbc123

bsclifton force-pushed the readd-widevine-gn-support branch from 2b10c76 to afbc123 Compare October 14, 2018 04:33

bsclifton added a commit to brave/brave-browser that referenced this pull request Oct 14, 2018

For testing with CI, set the brave-core branch to brave/brave-core#515

f0cb96e

bsclifton mentioned this pull request Nov 10, 2018

Check BUILDFLAG for Widevine #884

Merged

18 tasks

bbondy removed the pr/work-in-progress label Mar 1, 2019

mihaiplesa closed this Apr 3, 2019

bsclifton deleted the readd-widevine-gn-support branch April 3, 2019 22:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Update GN flags to properly do Widevine Host Verification / signing #515

WIP: Update GN flags to properly do Widevine Host Verification / signing #515

bsclifton commented Sep 27, 2018 •

edited

Loading

darkdh Sep 27, 2018

darkdh Sep 27, 2018

darkdh Sep 27, 2018

darkdh Sep 27, 2018 •

edited

Loading

darkdh Sep 27, 2018

darkdh commented Sep 28, 2018

bsclifton commented Sep 28, 2018

bbondy Oct 11, 2018

bbondy Oct 11, 2018

bsclifton Oct 12, 2018

bbondy commented Dec 2, 2018

mihaiplesa commented Apr 3, 2019

WIP: Update GN flags to properly do Widevine Host Verification / signing #515

WIP: Update GN flags to properly do Widevine Host Verification / signing #515

Conversation

bsclifton commented Sep 27, 2018 • edited Loading

Current Status

Windows Status

non-Windows builds register the UtilityServiceFactory

UtilityServiceFactory registers implementation for CdmService

The implementation for CdmService provides the host verification

What exactly is broken on Windows?

What is left? Where is help needed?

Why does Chrome work?

Description

Submitter Checklist:

Test Plan:

Reviewer Checklist:

darkdh Sep 27, 2018

Choose a reason for hiding this comment

darkdh Sep 27, 2018

Choose a reason for hiding this comment

darkdh Sep 27, 2018

Choose a reason for hiding this comment

darkdh Sep 27, 2018 • edited Loading

Choose a reason for hiding this comment

darkdh Sep 27, 2018

Choose a reason for hiding this comment

darkdh commented Sep 28, 2018

bsclifton commented Sep 28, 2018

bbondy Oct 11, 2018

Choose a reason for hiding this comment

bbondy Oct 11, 2018

Choose a reason for hiding this comment

bsclifton Oct 12, 2018

Choose a reason for hiding this comment

bbondy commented Dec 2, 2018

mihaiplesa commented Apr 3, 2019

bsclifton commented Sep 27, 2018 •

edited

Loading

darkdh Sep 27, 2018 •

edited

Loading